-
-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move to new ld client package, import package via ember-auto import #133
Move to new ld client package, import package via ember-auto import #133
Conversation
Code looks good to me - can we get this merged @achambers or whoever else is it may concern? |
Thanks for this @chrisvdp . I'll be casting some focus towards this this week. There are few things I need to verify here before merging due to how far behind we are with the client lib version. Between @mansona and myself, hopefully we can get some traction on this by the end of the week. /cc @ToMoCoop |
@achambers sounds good. Let me know if there are any tasks I can take on to help. |
@mansona @achambers anything I can do to help get these LD client update PRs in? |
@achambers any updates? |
@achambers @mansona, just checking back in here. Anything I can do to help with this PR? I've got some time to devote to the project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all in all I'm in favour of this change, bring on the future 🎉
I have 2 suggestions now that we have changed how this works 👍 one of which is mostly a style thing ;) but there are some benefits to it 😂
Firstly, massive apologies for being so crappy with getting to this. Has anyone been through the release notes to see what has changed that we might need to be aware of? I did this quite some time ago and there were a handful of things that felt like should be considered before/as a part of upgrading. Just looking through them again now and these things feel like we need to, at the very least, consider whether there is something we need to take in to account... 2.0.0
The fact that 2.7.0
This repo currently waits 10 seconds for the "ready" event and rejects if there is an issue. Feels like we should move to using this 2.8.0
Do we need to understand and consider this? This repo supports streaming. I'm not fully clear on this change and am unsure if we need to consider it. Decide/understand whether this is relevant to this PR Other thoughts
Ok, so there's not as much as I first thought. Still, what are people's thoughts on these things and whether or not we need to consider them as a part of this PR? /cc @mansona |
8498405
to
e33f234
Compare
f58b637
to
24cb54c
Compare
I don't see any blockers in the updates identified in these release notes. I'd personally prefer to address any changes that may be desirable as a result of these updates in follow up PRs. I'm happy to take on the work of making any changes that are desired or necessary. I am hopeful that the forward momentum we'll gain by landing this PR will be a catalyst for fully updating the addon in the near future. Happy to reconsider if someone sees something that is problematic enough to need addressing now. |
Co-authored-by: Jared Galanis <[email protected]>
Co-authored-by: Chris Manson <[email protected]>
- removes dead code path for LDClient on window - uses imported initialize directly rather than calling it on the client
24cb54c
to
9cf1744
Compare
Hello, thanks for doing the leg work to get this repo setup!
We required the latest launch-darkly package for our internal product, so I thought I would share the changes I made via PR.
launchdarkly-js-client-sdk
and using the es6 import syntax instead of the global scope.If this gets merged, there are some subsequent changes to the code that allows for anonymous users that I can push.
cc: @mansona